-
-
Notifications
You must be signed in to change notification settings - Fork 255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix issue #2932 (allow speculative nested variables) #2940
Conversation
gen/nested.cpp
Outdated
@@ -84,11 +100,11 @@ DValue *DtoNestedVariable(Loc &loc, Type *astype, VarDeclaration *vd, | |||
IF_LOG { Logger::cout() << "Context: " << *ctx << '\n'; } | |||
|
|||
DtoCreateNestedContextType(vdparent->isFuncDeclaration()); | |||
assert(isIrLocalCreated(vd)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@klickverbot: I guess it would be safer to do the check after the DtoCreateNestedContextType()
call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, it's been a while – I'd need to look into this myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, I changed it to the safe variant.
auto llType = DtoPtrToType(astype); | ||
if (isSpecialRefVar(vd)) | ||
llType = llType->getPointerTo(); | ||
return makeVarDValue(astype, vd, llvm::ConstantPointerNull::get(llType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure as to whether null
or undefined
makes more sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't expect it to be used (or only from unused code), I'd use undef
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh yeah, those functions shouldn't be called, but we could just as well have a bug somewhere leading to this, where we wouldn't bail out anymore. I now lean more towards null
for deterministic segfaults; e.g., the std.random
2.084 issue (undef
initial seed) was only noticeable on 32-bit Linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using null
would not guarantee segfaults, but it clearly defines accessing it as UB (unless with "null-pointer-is-valid"=true
, but that would then result in segfaulting). I don't think that dereferencing undef
is UB in LLVM IR. So I too think null
is the better choice here (optimizer can perhaps also use it e.g to remove dead code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
Are we okay with this? We probably all agree that such functions shouldn't be emitted in the first place, but in the meantime, this hopefully allows people to play around with |
No description provided.